Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add fonts to support more different types of characters for multiple languages #29861

Merged
merged 10 commits into from
Jan 28, 2022

Conversation

lucasfcosta
Copy link
Contributor

@lucasfcosta lucasfcosta commented Jan 14, 2022

⚠️ Please let me know if we should backport this to 7.17.

What does this PR do?

This PR fixes #29495 by adding more fonts to support multiple different characters from multiple languages.

After upgrading the base image, even though all characters from a few languages worked fine, not all characters from all languages were being displayed.

Therefore, this PR installs the following packages within the elastic-agent and heartbeat images so that those can work fine:

fonts-arphic-ukai
fonts-arphic-uming
fonts-ipafont-mincho
fonts-ipafont-gothic
fonts-unfonts-core

Why is it important?

This is important because customer's websites use a wide variety of language and we must have an inclusive synthetics product capable of displaying a wide variety of characters in the screenshots it takes.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Author's Checklist

I'd appreciate it if reviewers could check the following items:

  • Do these dependencies cover all characters we're planning on supporting?
  • Are there more font-packs we need? If so, which?
  • Does this need a backport for 7.17?

How to test this PR locally

For Heartbeat

  1. Checkout this branch
  2. Navigate into the heartbeat directory within x-pack
  3. Build its image using env PLATFORMS="+all linux/amd64" mage package.
  4. Create a file with monitors which access websites in different languages:
    output.elasticsearch:
      hosts: ["http://elasticsearch:9200"]
      username: "elastic"
      password: "changeme"
    
    heartbeat.monitors:
    - type: browser
      id: korean-characters
      name: korean_characters_test
      schedule: '@every 1m'
      source:
        inline:
          script: |-
            step("first", async () => {
              await page.goto('https://www.naver.com/');
            });
    - type: browser
      id: chinese-characters
      name: chinese_characters_test
      schedule: '@every 1m'
      source:
        inline:
          script: |-
            step("first", async () => {
              await page.goto('https://baike.baidu.com/');
            });
    - type: browser
      id: russian-characters
      name: russian_characters_test
      schedule: '@every 1m'
      source:
        inline:
          script: |-
            step("first", async () => {
              await page.goto('https://yandex.ru/');
            });
    - type: browser
      id: arabic-characters
      name: arabic_characters_test
      schedule: '@every 1m'
      source:
        inline:
          script: |-
            step("first", async () => {
              await page.goto('https://www.bbc.com/arabic');
            });
  5. Run elastic-package stack up and then run the Heartbeat container mounting the configuration file above. That will cause the container to use the locally built image.
    docker run \
      -it \
      -u 0 \
      --entrypoint=/bin/bash \
      --rm \
      --name=heartbeat \
      --user=heartbeat \
      --volume="/tmp/hb1.yml:/usr/share/heartbeat/heartbeat.yml:ro" \
      --network="elastic-package-stack_default" \
      docker.elastic.co/beats/heartbeat:8.1.0
    
  6. Check the screenshot which Heartbeat is going to send for each inline monitor.

For Elastic Agent

  1. Checkout this branch
  2. Navigate into the elastic-agent directory within x-pack
  3. Build its image using env PLATFORMS="+all linux/amd64" mage dev:package.
  4. Start an Elastic Stack using elastic-package stack up using a compatible stack version
  5. In Kibana, create browser monitors which access the following websites:
    step("first", async () => {
      await page.goto('https://www.naver.com/');
    });
    
  6. Run a container with the elastic-agent-complete image and change its entry-point to /bin/bash so you can go into it.
    docker run \
      -it \
      --entrypoint=/bin/bash \
      --rm \
      --name=elastic-agent \
      --user=elastic-agent \
      --volume="/Users/lucasfcosta/Repositories/synthetics:/usr/share/synthetics" \
      --network="elastic-package-stack_default" \
      docker.elastic.co/beats/elastic-agent-complete:8.1.0
    
  7. Enroll your agent with elastic-agent enroll --insecure --url=http://fleet-server:8220 --enrollment-token=POLICY_TOKEN_HERE. You can get the enrollment token and policy tokens in Kibana (under Fleet).
  8. Start your agent with elastic-agent & and wait for it to send screenshots for the monitors you've set up in Kibana.

Related issues

Screenshots

Screenshot 2022-01-14 at 14 31 41
Screenshot 2022-01-14 at 14 32 05
Screenshot 2022-01-14 at 14 31 51
Screenshot 2022-01-14 at 14 32 15

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Jan 14, 2022
@mergify
Copy link
Contributor

mergify bot commented Jan 14, 2022

This pull request does not have a backport label. Could you fix it @lucasfcosta? 🙏
To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-v./d./d./d is the label to automatically backport to the 7./d branch. /d is the digit

NOTE: backport-skip has been added to this pull request.

@mergify mergify bot added the backport-skip Skip notification from the automated backport with mergify label Jan 14, 2022
@lucasfcosta lucasfcosta added the Team:obs-ds-hosted-services Label for the Observability Hosted Services team label Jan 14, 2022
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Jan 14, 2022
@lucasfcosta lucasfcosta added backport backport-v8.0.0 Automated backport with mergify backport-v8.1.0 Automated backport with mergify labels Jan 14, 2022
@mergify mergify bot removed the backport-skip Skip notification from the automated backport with mergify label Jan 14, 2022
@andrewvc
Copy link
Contributor

LGTM, tested all those sites myself, they all work

@lucasfcosta lucasfcosta marked this pull request as ready for review January 14, 2022 16:53
@lucasfcosta lucasfcosta requested a review from a team as a code owner January 14, 2022 16:53
@elasticmachine
Copy link
Collaborator

Pinging @elastic/uptime (Team:Uptime)

@elasticmachine
Copy link
Collaborator

elasticmachine commented Jan 14, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Reason: null

  • Start Time: 2022-01-28T09:09:44.611+0000

  • Duration: 220 min 3 sec

  • Commit: 668449d

Test stats 🧪

Test Results
Failed 0
Passed 47979
Skipped 4285
Total 52264

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@@ -69,7 +69,12 @@ RUN apt-get update -y && \
libcairo2\
libasound2\
libatspi2.0-0\
libxshmfence1 && \
libxshmfence1 \
fonts-arphic-ukai \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am curios about the list we arrived at here, We don't even have the basic one fonts-liberation for the image.

Do we know if that works?

Here is the list from PW for comparison - https://github.com/microsoft/playwright/blob/5ba7903ba098586a13745e0d7ac894f1d55d47aa/packages/playwright-core/src/utils/nativeDeps.ts#L434-L470

One we use in RUM agent for puppeteer and playwright - https://github.com/elastic/apm-agent-rum-js/tree/master/.ci/docker

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I followed an approach of testing websites and taking note of which didn't display all characters correctly. Then, I went after the packages only for those, and ran an E2E test which generated the images above. I followed this process to avoid adding too many unnecessary packages.

Wrt to fonts-liberation, it seemed to be a standard set of fonts which could just replace the default set of fonts to which it's compatible, but we already have default fonts for western characters in the base image and thus I thought we wouldn't need that. Looking into the other packages, they also seem to fall into one of two categories:

  • Covering standard character sets (like fonts-liberation or ttf-unifont)
  • Tools for managing fonts themselves (like libfontconfig)

The other fonts seemed to be the most complete and popular fonts for their character standards (fonts-arphic-ukai [for chinese characters] , fonts-ipafont-mincho [for japanese characters, fonts-ipafont-gothic being for sans-serif japanese characters], and fonts-unfonts-core for Korean characters). Those were the ones which came up when I looked for packages for each individual language.

We already had support for Arabic/Russian characters in the base image as per the initial test, so I haven't added those as the websites I visited already displayed correctly. It could be the case that there are characters outside the unicode space

Looking at Playwright's deps these are the changes I see we could make:

I moved this PR to draft just so that we would make sure to review the licensing and whether we're happy with the current coverage set. I do thing we should add fonts-tlwg though, so I'll run a test for that before opening this PR for review again too.

@andrewvc
Copy link
Contributor

/test

Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm LGTM but let's address @vigneshshanmugam 's comments before merging

@lucasfcosta
Copy link
Contributor Author

After talking about this issue with @vigneshshanmugam, I've addressed a couple more of his points:

  • I double-checked that the new image has good support for default fallback fonts. I did so by accessing different websites with all requests enabled and then accessing the very same websites while disabling font requests in playwright.
    Here is an example of a script I used.
    step("cnn without fonts", async () => {
        await page.route('**/*', (route) => {
              return route.request().resourceType() === 'font'
                  ? route.abort()
                  : route.continue()
        });
        await page.goto('https://cnn.com');
    });
    
    step("cnn with fonts", async () => {
        await page.route('**/*', (route) => route.continue());
        await page.goto('https://cnn.com');
    });
    I also accessed https://furbo.org/stuff/systemfonts-new.html, which has default font values in CSS and loads no font files. In that website, all of the pieces of text rendered fine in the system's fallback font (although they don't have stylistic differences as we're using a fallback).
  • Furthermore, I also confirmed that we cannot run headful tests in the new ubuntu images because there's no X display in them (as shown in the error log below).
    heartbeat@0699f71daf07:~$ synthetics /usr/share/synth-example/ --no-headless
      Failed to run the test browserType.launch: Protocol error (Browser.getVersion): Browser closed.
      ==================== Browser output: ====================
      <launching> /usr/share/heartbeat/.cache/ms-playwright/chromium-907428/chrome-linux/chrome --disable-background-networking --enable-features=NetworkService,NetworkServiceInProcess --disable-background-timer-throttling --disable-backgrounding-occluded-windows --disable-breakpad --disable-client-side-phishing-detection --disable-component-extensions-with-background-pages --disable-default-apps --disable-dev-shm-usage --disable-extensions --disable-features=ImprovedCookieControls,LazyFrameLoading,GlobalMediaControls,DestroyProfileOnBrowserClose --allow-pre-commit-input --disable-hang-monitor --disable-ipc-flooding-protection --disable-popup-blocking --disable-prompt-on-repost --disable-renderer-backgrounding --disable-sync --force-color-profile=srgb --metrics-recording-only --no-first-run --enable-automation --password-store=basic --use-mock-keychain --no-service-autorun --user-data-dir=/tmp/playwright_chromiumdev_profile-vWKBgW --remote-debugging-pipe --no-sandbox --no-startup-window
      <launched> pid=244
      [pid=244][err] [244:244:0119/160156.284241:ERROR:browser_main_loop.cc(1400)] Unable to open X display.
      =========================== logs ===========================
      <launching> /usr/share/heartbeat/.cache/ms-playwright/chromium-907428/chrome-linux/chrome --disable-background-networking --enable-features=NetworkService,NetworkServiceInProcess --disable-background-timer-throttling --disable-backgrounding-occluded-windows --disable-breakpad --disable-client-side-phishing-detection --disable-component-extensions-with-background-pages --disable-default-apps --disable-dev-shm-usage --disable-extensions --disable-features=ImprovedCookieControls,LazyFrameLoading,GlobalMediaControls,DestroyProfileOnBrowserClose --allow-pre-commit-input --disable-hang-monitor --disable-ipc-flooding-protection --disable-popup-blocking --disable-prompt-on-repost --disable-renderer-backgrounding --disable-sync --force-color-profile=srgb --metrics-recording-only --no-first-run --enable-automation --password-store=basic --use-mock-keychain --no-service-autorun --user-data-dir=/tmp/playwright_chromiumdev_profile-vWKBgW --remote-debugging-pipe --no-sandbox --no-startup-window
      <launched> pid=244
      [pid=244][err] [244:244:0119/160156.284241:ERROR:browser_main_loop.cc(1400)] Unable to open X display.
      ============================================================
          at Function.setupDriver (/usr/share/heartbeat/.node/node/lib/node_modules/@elastic/synthetics/src/core/gatherer.ts:48:43)
          at Function.createContext (/usr/share/heartbeat/.node/node/lib/node_modules/@elastic/synthetics/src/core/runner.ts:157:35)
          at Runner.runJourney (/usr/share/heartbeat/.node/node/lib/node_modules/@elastic/synthetics/src/core/runner.ts:410:34)
          at Runner.run (/usr/share/heartbeat/.node/node/lib/node_modules/@elastic/synthetics/src/core/runner.ts:488:22)
          at Object.run (/usr/share/heartbeat/.node/node/lib/node_modules/@elastic/synthetics/src/index.ts:45:12)
          at /usr/share/heartbeat/.node/node/lib/node_modules/@elastic/synthetics/src/cli.ts:201:19 {
        name: 'Error'
      }
    
    It's also worth highlighting that we were also not able to run headful tests on the old Heartbeat image too. I tried running Headful tests within the 7.16 image and got the same error.
    @vigneshshanmugam do you think that should be a supported case within the images? I feel like it's up to the user to install X and create the necessary display if they'd like to run headful tests, but I'm of course open to discussing it.
    Also, if we do end-up supporting headful runs, that means we will not disable GPU acceleration, which may cause failures inside these containers, so we'd need to handle that better with libmesa or one of the alternatives we discussed earlier.

@vigneshshanmugam
Copy link
Member

@lucasfcosta Thanks Lucas for testing it and providing the details.

In that website, all of the pieces of text rendered fine in the system's fallback font

Great.

do you think that should be a supported case within the images? I feel like it's up to the user to install X and create the necessary display if they'd like to run headful tests, but I'm of course open to discussing it.

Interesting, I didn't know the old image was not able to run headful tests. I am fine with any of the below options

  • Leave it up to the user to do the full setup.
  • We can add xvfb to the dependencies and let the user do the rest.

Also, if we do end-up supporting headful runs, that means we will not disable GPU acceleration, which may cause failures inside these containers, so we'd need to handle that better with libmesa or one of the alternatives we discussed earlier.

Gotcha, we should make a call here. Lets see what others have to say @andrewvc @paulb-elastic @drewpost

@drewpost
Copy link

@lucasfcosta Thanks Lucas for testing it and providing the details.

In that website, all of the pieces of text rendered fine in the system's fallback font

Great.

do you think that should be a supported case within the images? I feel like it's up to the user to install X and create the necessary display if they'd like to run headful tests, but I'm of course open to discussing it.

Interesting, I didn't know the old image was not able to run headful tests. I am fine with any of the below options

  • Leave it up to the user to do the full setup.
  • We can add xvfb to the dependencies and let the user do the rest.

Also, if we do end-up supporting headful runs, that means we will not disable GPU acceleration, which may cause failures inside these containers, so we'd need to handle that better with libmesa or one of the alternatives we discussed earlier.

Gotcha, we should make a call here. Lets see what others have to say @andrewvc @paulb-elastic @drewpost

My gut feel is that it's OK to disable GPU acceleration. We're not a device simulation testing product. What's key is to keep the results consistent and as long as we pick one option or the other and stick with it, then that requirement is met. There may be real world impact on the sizing we need to allow for given the more inefficient CPU only approach and I'm open to that as being something we discuss from an operation efficiency standpoint but from a product perspective, I'm OK with not enabling GPU acceleration. cc @lucasfcosta @vigneshshanmugam

@mergify
Copy link
Contributor

mergify bot commented Jan 25, 2022

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b install-fonts upstream/install-fonts
git merge upstream/master
git push upstream install-fonts

@lucasfcosta
Copy link
Contributor Author

Hey @drewpost thanks a lot for your input! 😊 With regards to your last point about GPU acceleration I've recently run some tests within containers and it seems like containers do not have access to the underlying host's GPU so there's no actual difference in performance.

I've added a few more details in this link.

@andrewvc
Copy link
Contributor

I've pushed a change to use google's SIL licensed noto fonts. All the tests listed previously still work, with all languages rendering.

@lucasfcosta lucasfcosta marked this pull request as ready for review January 27, 2022 09:25
@lucasfcosta
Copy link
Contributor Author

I have re-tested all the websites in this PR with the new fonts and they all worked fine, including Thai websites, which I hadn't checked before.

As long as everyone's happy with the chosen font's licenses, I think we can go ahead and merge this in. What do you think @andrewvc?

Screenshot 2022-01-27 at 14 28 46

@lucasfcosta
Copy link
Contributor Author

/test

Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

apt-get clean all && \
exit_code=0 && break || exit_code=$? && echo "apt-get error: retry $iter in 10s" && sleep 10; \
done; \
(exit $exit_code)
RUN apt install fonts-noto-cjk
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we explicitly install the fonts on elastic-agent image and not on the HB image?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, that's a bug, it's actually installed higher up, this is redundant

Copy link
Member

@vigneshshanmugam vigneshshanmugam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with pending comment #29861 (comment)

@andrewvc andrewvc merged commit 457b0bb into elastic:master Jan 28, 2022
mergify bot pushed a commit that referenced this pull request Jan 28, 2022
…languages (#29861)

This PR fixes #29495 by adding more fonts to support multiple different characters from multiple languages.

After upgrading the base image, even though all characters from a few languages worked fine, not all characters from all languages were being displayed.

(cherry picked from commit 457b0bb)
@lucasfcosta lucasfcosta deleted the install-fonts branch January 28, 2022 16:07
@andrewvc
Copy link
Contributor

@Mergifyio backport 7.17

mergify bot pushed a commit that referenced this pull request Jan 28, 2022
…languages (#29861)

This PR fixes #29495 by adding more fonts to support multiple different characters from multiple languages.

After upgrading the base image, even though all characters from a few languages worked fine, not all characters from all languages were being displayed.

(cherry picked from commit 457b0bb)
@mergify
Copy link
Contributor

mergify bot commented Jan 28, 2022

backport 7.17

✅ Backports have been created

andrewvc pushed a commit that referenced this pull request Jan 28, 2022
…languages (#29861) (#30091)

This PR fixes #29495 by adding more fonts to support multiple different characters from multiple languages.

After upgrading the base image, even though all characters from a few languages worked fine, not all characters from all languages were being displayed.

(cherry picked from commit 457b0bb)

Co-authored-by: Lucas F. da Costa <[email protected]>
andrewvc pushed a commit that referenced this pull request Jan 28, 2022
…languages (#29861) (#30093)

This PR fixes #29495 by adding more fonts to support multiple different characters from multiple languages.

After upgrading the base image, even though all characters from a few languages worked fine, not all characters from all languages were being displayed.

(cherry picked from commit 457b0bb)

Co-authored-by: Lucas F. da Costa <[email protected]>
v1v added a commit to v1v/beats that referenced this pull request Jan 31, 2022
…k-version-after-8-0-creation

* upstream/master: (69 commits)
  Update stale config following (elastic#30082)
  Make include_matches backwards compatible with 7.x config (elastic#30032)
  [Filebeat] Update handling of elasticsearch server logs (elastic#30018)
  Remove SSL3 support from libbeat and its documentation. (elastic#30071)
  Revert "Packaging: rename arm64 suffix to aarch64 in the tar.gz artifacts ONLY (elastic#28813)" (elastic#30083)
  [libbeat] Add script processor to all beats (elastic#29752)
  Add fonts to support more different types of characters for multiple languages (elastic#29861)
  libbeat/reader: Fix messge conversion to beat.Event (elastic#30057)
  probot[stale]: ignore issues with the tag flaky-test (elastic#30065)
  [DOCS] Add redirect for GSuite module (elastic#30034)
  [Automation] Update elastic stack version to 8.1.0-aa69d697 for testing (elastic#30012)
  Remove msitools install for windows build, using the latest docker image with msitools preinstalled (elastic#30040)
  filebeat/generator/fields: fix dropped error (elastic#29943)
  Include the error message with auditd module events (elastic#30009)
  [Metricbeat] gcp: add firestore metricset (elastic#29918)
  probot: update stale dates (elastic#29997)
  Metricbeat enterprise search module: add xpack.enabled support (elastic#29871)
  x-pack/packetbeat: install Npcap at start-up when required (elastic#29112)
  [Filebeat] Fix panic in decode_cef when recovering from invalid data (elastic#30038)
  Correctly fixe how selected packages are defined (elastic#30039)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport backport-v8.0.0 Automated backport with mergify backport-v8.1.0 Automated backport with mergify Team:obs-ds-hosted-services Label for the Observability Hosted Services team v7.17.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Heartbeat] Missing some i18n fonts in heartbeat docker image
5 participants